fix(webdriverjs): fix default commonJs export #927
Conversation
straker
force-pushed
the
webdriverjs-default-export
branch
from
97104ba
to
5425097
Compare
|
This PR touches a lot more than webdriverjs. May want to update the commit scope 🙂 |
There was a problem hiding this comment.
It might be nice to also include tests of importing from typescript; we've seen at least one case where a CJS/ESM-related misconfiguration resulted in a package that could be used successfully from JS, but resulted in an empty typings file that broke importing from typescript. Maybe something like:
// This file isn't executed; we only care that typescript can successfully
// build/typecheck. It is a smoke test that our build process is producing a
// basically-reasonable index.d.ts.
// Detailed tests of individual types should instead be covered by unit tests.
import { AxeBuilder as NamedImportAxeBuilder } from '../dist/index.js';
import DefaultImportAxeBuilder from '../dist/index.js';
import type { WebDriver } from 'selenium-webdriver';
// See https://stackoverflow.com/a/55541672
type IsAny<T> = 0 extends (1 & T) ? true : false;
// If the imports don't have typings assigned, these will fail
// with "ts(2322): Type 'false' is not assignable to type 'true'."
(x: IsAny<typeof NamedImportAxeBuilder>): false => x;
(x: IsAny<typeof DefaultImportAxeBuilder>): false => x;
new NamedImportAxeBuilder({} as WebDriver)
.withRules('color-contrast')
.analyze();
new DefaultImportAxeBuilder({} as WebDriver)
.withRules('color-contrast')
.analyze();
| @@ -291,4 +291,10 @@ export default class AxeBuilder { | |||
| } | |||
| } | |||
|
|
|||
| // ensure backwards compatibility with commonJs default export | |||
| if (typeof module === 'object') { | |||
| module.exports = AxeBuilder; | |||
There was a problem hiding this comment.
It's unfortunate that this causes some warning spew in the build:
warning spew
/repos/axe-core-npm/packages/webdriverjs (webdriverjs-default-export*) » yarn build danbjorge@Dans-MacBook-Pro
yarn run v1.22.19
$ rimraf dist
$ tsup src/index.ts --dts --format esm,cjs
CLI Building entry: src/index.ts
CLI Using tsconfig: tsconfig.json
CLI tsup v6.7.0
CLI Target: esnext
ESM Build start
CJS Build start
WARN ▲ [WARNING] The CommonJS "module" variable is treated as a global variable in an ECMAScript module and may not work as expected [commonjs-variable-in-esm]
src/index.ts:296:2:
296 │ module.exports = AxeBuilder
╵ ~~~~~~
This file is considered to be an ECMAScript module because of the "export" keyword here:
src/index.ts:301:0:
301 │ export { AxeBuilder };
╵ ~~~~~~
WARN ▲ [WARNING] The CommonJS "module" variable is treated as a global variable in an ECMAScript module and may not work as expected [commonjs-variable-in-esm]
src/index.ts:296:2:
296 │ module.exports = AxeBuilder
╵ ~~~~~~
This file is considered to be an ECMAScript module because of the "export" keyword here:
src/index.ts:301:0:
301 │ export { AxeBuilder };
╵ ~~~~~~
ESM dist/index.mjs 14.24 KB
ESM ⚡️ Build success in 39ms
CJS dist/index.js 15.97 KB
CJS ⚡️ Build success in 39ms
DTS Build start
^[[A^[[ADTS ⚡️ Build success in 809ms
DTS dist/index.d.ts 2.52 KB
✨ Done in 1.27s.
This is a reasonable warning, but one that we reasonably want to suppress here, I think. I didn't see a good way of doing that - it comes from esbuild (not eslint), which doesn't support a suppress-next-line type comment. esbuild does have a --log-override option to quiet it, but only for the whole build process, not for a few lines, and tsup doesn't support forwarding it anyway (they tried but encountered some issues getting it to work and abandoned it :( )
I don't think it's worth more time trying to suppress the warning, just noting it here so noone repeats the same research.
|
I'm going to split the TS type check and the support for default/ |
This fixes the breaking change we accidentally released in 4.7.3 when we broke the default export of
@axe-core/webdriverjs. Also added tests to ensure we don't make the same mistake again.Closes #940